Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Re-introduce authn/authz for metrics endpoint #3968

Conversation

joelanford
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2024
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 3, 2024
@joelanford joelanford force-pushed the authn-authz-controller-runtime-metrics branch from e2b8a3a to 7797875 Compare June 3, 2024 21:40
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford joelanford force-pushed the authn-authz-controller-runtime-metrics branch from 7797875 to 15d8fa6 Compare June 3, 2024 22:17
SecureServing: secureMetrics,
TLSOpts: tlsOpts,
BindAddress: metricsAddr,
FilterProvider: filters.WithAuthenticationAndAuthorization,
Copy link
Member

@camilamacedo86 camilamacedo86 Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we have an option to give the users the choice to opt-in or not to use this one?
What about we scaffold it commented and add comments in the code to allow people opt-in to use it if they wish OR we have a plugin that do it ?

I am not sure what would be the best I just felt that now that finally we clean it up we have the chance to do that properly and give the chance for the users opt-in or not so that we are able to properly address the feedbacks. WDYT?

Copy link
Member

@camilamacedo86 camilamacedo86 Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you good idea as always :-) ?

So users reported that they do not like kube-rbac-proxy in the past and would like to not see that by default (i.e. #3482) others shared that mainly only use network-policies so I was wonder if now we could grow it in a way that give more power to users opt-in or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the two ideal states are:

  • Metrics listening on localhost only
  • Metrics listening via TLS with a valid cert chain and this (or a similar) filter.

Do you agree with that? If so, I think that means that the cert-manager overlay would actually be the thing that controls the flags that are passed into the controller container to enable the TLS listen address and the authn/authz filters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I totally agree.
I think the next step would be add an option to patch the cert-manager config as well

# If you want to expose the metric endpoint of your controller-manager uncomment the following line.
#- path: manager_metrics_patch.yaml
# target:
# kind: Deployment
Copy link
Member

@camilamacedo86 camilamacedo86 Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not keep this one so that we allow people disable if they wish?
We can let enable with :8443 instead of :8080 by default

if _, err := t.Kubectl.WithInput(string(output)).Command("delete", "-f", "-"); err != nil {
warnError(err)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 good catcher

}

// GenerateV4WithoutMetrics implements a go/v4 plugin project defined by a TestContext.
func GenerateV4WithoutMetrics(kbc *utils.TestContext) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think would be great we keep a test to ensure that the metrics are not exposed and we have an option to opt-out from it that works.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Contributor

@Kavinjsir Kavinjsir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@@ -70,17 +70,11 @@ resources:
#- ../certmanager
# [PROMETHEUS] To enable prometheus monitor, uncomment all sections with 'PROMETHEUS'.
#- ../prometheus
# [METRICS] To enable the controller manager metrics service, uncomment the following line.
#- metrics_service.yaml
# [METRICS] Expose the controller manager metrics service.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:
We can probably remind users of role binding for scraping, such as:

# To scrape metrics e.g. via Prometheus the client needs the ClusterRole `metrics-reader`

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford, Kavinjsir

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2024
@camilamacedo86
Copy link
Member

Hi @joelanford

AS we spoke I am closing this one in favor of #4003
which has your changes rebased with the master and some improvements on top.

Again, thank you a lot for your contribution and amazing help.!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants